Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update hyper to 1.x and integrate with hyper-util #232

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Update hyper to 1.x and integrate with hyper-util #232

merged 2 commits into from
Jan 11, 2024

Conversation

Gelbpunkt
Copy link
Contributor

@Gelbpunkt Gelbpunkt commented Nov 18, 2023

Since hyper 1.x leaves control of the accepting process to the user (explanation), I removed the acceptor API and server examples.

The tokio feature in hyper-util is required for the IO adapter traits, so the need for a tokio-runtime feature is gone (it was already a hard dependency before).

@Gelbpunkt Gelbpunkt marked this pull request as draft November 18, 2023 03:20
@cpu cpu mentioned this pull request Nov 27, 2023
@Gelbpunkt Gelbpunkt marked this pull request as ready for review January 4, 2024 23:19
@Gelbpunkt
Copy link
Contributor Author

I marked this as ready for review but since I still didn't hear back regarding the future of the acceptor API, it isn't really "ready". If it were up to me, I'd completely remove the acceptor API, but I'm not the one to make that decision so I'd really prefer hearing from the maintainers.

This PR is blocking a lot of upgrades to hyper 1.0 in the ecosystem, I'd really appreciate hearing back so this PR can be reviewed properly.

4t145 added a commit to ideal-world/spacegate that referenced this pull request Jan 5, 2024
@cpu
Copy link
Member

cpu commented Jan 5, 2024

Thanks for the PR. This is a substantial update and I'm not as familiar with the guts of hyper-rustls as I am with other parts of the Rustls ecosystem. Spinning up on the Hyper 1.0 API changes and the work in this branch will take time that I haven't been able to find yet. Your patience is appreciated.

If it were up to me, I'd completely remove the acceptor API

Have you looked at any downstream consumers of this crate to see if they are using the acceptor API, and what they would need to do in place of it were it removed?

@Gelbpunkt
Copy link
Contributor Author

Gelbpunkt commented Jan 5, 2024

Have you looked at any downstream consumers of this crate to see if they are using the acceptor API, and what they would need to do in place of it were it removed?
Thing is, the acceptor trait was removed from hyper entirely. There is no more direct purpose in having something that previously implemented the trait living in hyper-rustls anymore. hyper pre-1.0 had a high-level server API that would use the trait, this high-level API was removed without replacement. In hyper 1.0, users have to create the TcpListener themselves and drive the HTTP connection on the TcpStream themselves.

Let me demonstrate what a hyper server looks like without TLS in 0.14 and 1.x so I can better explain the changes to the accept process.

// hyper 0.14
use std::convert::Infallible;
use std::net::SocketAddr;
use hyper::{Body, Request, Response, Server};
use hyper::service::{make_service_fn, service_fn};

async fn handle(_req: Request<Body>) -> Result<Response<Body>, Infallible> {
    Ok(Response::new(Body::from("Hello World")))
}

#[tokio::main]
async fn main() {
    // Construct our SocketAddr to listen on...
    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));

    // And a MakeService to handle each connection...
    let make_service = make_service_fn(|_conn| async {
        Ok::<_, Infallible>(service_fn(handle))
    });

    // Then bind and serve...
    let server = Server::bind(&addr).serve(make_service);

    // And run forever...
    if let Err(e) = server.await {
        eprintln!("server error: {}", e);
    }
}

And here's the hyper 1.x equivalent:

// Hyper 1.x
use http_body_util::Full;
use hyper::body::{Bytes, Incoming};
use hyper::service::service_fn;
use hyper::{Request, Response};
use hyper_util::rt::{TokioExecutor, TokioIo};
use hyper_util::server::conn::auto;
use std::convert::Infallible;
use std::net::SocketAddr;
use tokio::net::TcpListener;

async fn handle(_req: Request<Incoming>) -> Result<Response<Full<Bytes>>, Infallible> {
    Ok(Response::new(Full::from("Hello World")))
}

#[tokio::main]
async fn main() {
    // Construct our SocketAddr to listen on...
    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));

    // Bind to the address
    let listener = TcpListener::bind(addr).await.unwrap();

    // Serve incoming connections
    loop {
        let (stream, _) = listener.accept().await.unwrap();

        // Note that we have to drive the connections ourselves and also take care of the accepting process
        tokio::spawn(async move {
            auto::Builder::new(TokioExecutor::new())
                .serve_connection(TokioIo::new(stream), service_fn(handle))
                .await
                .unwrap();
        });
    }
}

You'll notice that the accepting of connections is now no longer up to hyper's now-removed Server, but rather the user. That is also why the Acceptor trait is removed: There is no more need for it. It was needed to customize the accept process of the Server, but now the user is in charge of this.

All that the hyper-rustls TlsAcceptor did was wrap the I/O received from hyper after calling accept in a TlsStream (relevant line). That is something that users can just do themselves now after they have accepted the connection. There's nothing hyper-specific about this anymore, therefore it doesn't need to reside in hyper-rustls anymore in my opinion.

I hope that somewhat explains why I think this is pointless now. Any "new" implementation of TlsAcceptor like the one I currently provide in this PR is quite literally a fancy wrapper over TlsStream::new coupled with a builder for Arc<ServerConfig>. I don't see why that should exist anymore.

@kurtbuilds
Copy link

kurtbuilds commented Jan 6, 2024

@Gelbpunkt Want to say thank you for putting the PR together.

I'm chiming in to confirm, I've got tons of code that is blocked on upgrading http=1.0 (and axum=0.7, etc) because this isn't merged, and I'm looking forward to seeing it happen.

I'm not that familiar with the guts of hyper, but agree, my understanding is that Acceptor is no longer a concept, and thus doesn't need migrating.

@cpu
Copy link
Member

cpu commented Jan 8, 2024

Let me demonstrate what a hyper server looks like without TLS in 0.14 and 1.x so I can better explain the changes to the accept process.

Thank you, this was helpful 👍

I don't see why that should exist anymore.

I'm convinced. I think you should go ahead and remove it from this branch (and update the PR description).

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some questions/small comments from a first pass. Thanks!

src/connector.rs Show resolved Hide resolved
src/connector.rs Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
@Gelbpunkt
Copy link
Contributor Author

One issue that arises with removing the server example is that there's no longer a test that runs the client example with custom certificates. I assume the best way forward is to inline a server that does this into the tests? I'm thinking that it seems a bit weird that the server and the client were testing "each other" previously, since a misimplementation might go unnoticed if it's in hyper-rustls. Wouldn't it be better to test the client's custom certificate with a completely different server implementation (similar to how the server was tested with curl)?

@cpu
Copy link
Member

cpu commented Jan 8, 2024

Looking good :-)

One issue that arises with removing the server example is that there's no longer a test that runs the client example with custom certificates.

Could this issue and @Arnavion's feedback be addressed at the same time by adding an example server that doesn't use the now-removed Acceptor abstraction, but runs an HTTPS server looping over accepting connections from a TcpListener? That is, keep the example, but remove only the Acceptor specific parts.

I'm thinking that it seems a bit weird that the server and the client were testing "each other" previously, since a misimplementation might go unnoticed if it's in hyper-rustls. Wouldn't it be better to test the client's custom certificate with a completely different server implementation (similar to how the server was tested with curl)?

I think it's fair to think about this as follow-up work and not block the hyper update on improving the tests like this.

rustls / Check semver compatibility (pull_request) Failing after 24s

If you want to update the Cargo.toml version to 0.26.0 this should resolve this CI task failing. Squashing your commit history would also be appreciated. Ideally this branch would be two commits: all of the existing changes as one Hyper update commit, and then the Cargo.toml version update as a second commit.

@kurtbuilds
Copy link

kurtbuilds commented Jan 8, 2024

One note, tower-service 0.3 depends on http 0.2. This fact doesn't block this work at all, but it does indicate that, to drop the http 0.2 dependency, we need either an updated hyper-util that depends on hyper::Service rather than tower-service::Service (I'm guessing this is preferable, but I don't know if it's feasible), or an updated tower-service that depends on http=1.0.

EDIT: Apologies. As pointed out below, http 0.2 is a dev-dependency of tower-service, thus not relevant.

@Gelbpunkt
Copy link
Contributor Author

One note, tower-service 0.3 depends on http 0.2. This fact doesn't block this work at all, but it does indicate that, to drop the http 0.2 dependency, we need either an updated hyper-util that depends on hyper::Service rather than tower-service::Service (I'm guessing this is preferable, but I don't know if it's feasible), or an updated tower-service that depends on http=1.0.

That is a dev-dependency and therefore not relevant for us.

@Gelbpunkt Gelbpunkt changed the title Update hyper to 1.0 and use hyper-util Update hyper to 1.x and integrate with hyper-util Jan 8, 2024
@Gelbpunkt
Copy link
Contributor Author

Gelbpunkt commented Jan 8, 2024

If you want to update the Cargo.toml version to 0.26.0 this should resolve this CI task failing. Squashing your commit history would also be appreciated. Ideally this branch would be two commits: all of the existing changes as one Hyper update commit, and then the Cargo.toml version update as a second commit.

Should be done. Earlier CI was failing because of the missing required-features on the revamped server example, that should now hopefully also pass. If not, I'll have to add some #[cfg(feature = "ring")] bounds to the tests...

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🚀

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this and sorry for taking so long to review this in more depth.

I think this is looking pretty good but still have few comments.

src/stream.rs Show resolved Hide resolved
src/stream.rs Outdated Show resolved Hide resolved
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two more tiny nits, but otherwise this looks good to me!

src/connector.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
This updates the entire dependency stack to hyper 1.x and http 1.x.

The tokio-runtime feature was removed since it no longer serves a
purpose and tokio is a hard dependency.

Since hyper 1.x removed the high-level server API that supported
customizing the accept process of the server via an Accept trait, the
acceptor in hyper-rustls is removed. The server example however was
updated to showcase how rustls can be integrated with the low-level
hyper 1.x API.

Signed-off-by: Jens Reidel <adrian@travitia.xyz>
Signed-off-by: Jens Reidel <adrian@travitia.xyz>
@cpu cpu enabled auto-merge January 11, 2024 18:41
@cpu cpu added this pull request to the merge queue Jan 11, 2024
Merged via the queue into rustls:main with commit ad93d22 Jan 11, 2024
11 checks passed
@cpu
Copy link
Member

cpu commented Jan 11, 2024

Thanks again @Gelbpunkt 🙇

@cpu
Copy link
Member

cpu commented Jan 11, 2024

Tracking a release with this work in #250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants